Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF, BRK] Decision tree modularization #476

Conversation

handwerkerd
Copy link
Member

Closes #403 .

Changes proposed in this pull request:

  • The decision tree steps in selection are modularized in new functions and classes
  • decision_tree_class.py contains the new class for initializing and running a decision tree
  • selection/data/ contains json files that define a simpler, conservative decision tree and the full MEICA v2.7 decision tree
    -Most functions that define each decision tree step are in selection_nodes.py
    -There are some minor changes in decision tree steps that might result in breaking the existing tests

Known issues:

  • Some functions in the MEICA decision tree haven't been run yet because they require linking to metric calculation functions. Those functions and all of PR [REF] Modularize metric calculation #447 is already merged into this code, to make that possible.
  • The function documentation isn't being linked to and generated in the ReadTheDocs API section. The documentation also has some gaps. In particular, the documentation for the class structure is not done and there should be a guide explaining the json files for the decision trees and listing rules for creating a new decision tree (i.e. Even though it is possible, it is not advised to create a decision tree that can re-classify accepted or rejected components once they've received those labels)
  • More checks and warnings can be added to various functions. For example, there can be a warning if an accepted or rejected component is reclassified at a later step
  • There is repetitive code at the beginning of each decision tree function. That can be changed into an initialization function (or all the functions in selection_nodes.py could become a class, but that might be more hassle than it's worth). Either of these changes probably wouldn't make the code much shorter, but it would make it easier to know what is expected for each function
  • Some of the functions for the later steps in the decision tree are ugly (i.e. highvariance_highmeanmetricrank_highkapparatio ). These are all functions that tweak metrics, threshold each of them separately, and use the intersection of those thresholded values to make a decision. I can theoretically make a multiple threshold comparison function that would be more generally useful. The minus is that would either break the current way decision nodes are set up, or push a lot more details and function calls into a decision node specification in the json file. On balance, I think these long functions with explanations of what they do are better than adding complexity to the json files, but this deserves a bit more thought. Also, I'm hoping we'll move away from decision trees with some messy joint measurements so I hope the currently messy functions will become legacy functions to maintain a rough replication of the MEICA method, but won't spawn too many additional messy functions.
  • No testing functions have been written for this code yet
  • All the other improvements that aren't coming to mind right now.

jbteves and others added 30 commits November 6, 2019 14:42
list should be used as a short-hand for common trees.
"""

fname = resource_filename('tedana', 'selection/data/{}.json'.format(tree))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to optionally support full paths here, so people can feed in their own trees with a full file path or they can call our built-in trees with just the name of the tree.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added support for full paths (and changed the error messages accordingly)

@tsalo
Copy link
Member

tsalo commented Nov 23, 2019

There are a lot of merge conflicts that will need to be address. Also, if you remove the pytest_cache files and revert changes to tedica.py (which I don't think your enhancements cover), that would make reviewing easier.

@emdupre
Copy link
Member

emdupre commented Nov 23, 2019

Also, if you remove the pytest_cache files and revert changes to tedica.py (which I don't think your enhancements cover), that would make reviewing easier.

+1, here. I'm not planning to review this until #475 is merged so the diff is minimized -- hopefully that, removing the cache files, and reverting tedica changes should make this easier !

@stale
Copy link

stale bot commented Mar 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to tedana:tada: !

@stale stale bot added the stale label Mar 1, 2020
@handwerkerd
Copy link
Member Author

Not stale. Paused until #475 is reviewed & ready to integrate with this PR.

@stale
Copy link

stale bot commented May 31, 2020

This issue has been automatically marked as stale because it has not had any activity in 90 days. It will be closed in 600 days if no further activity occurs. Thank you for your contributions to tedana:tada: !

@stale stale bot added the stale label May 31, 2020
@handwerkerd handwerkerd removed the stale label Jul 15, 2020
@tsalo tsalo closed this Aug 7, 2020
@tsalo
Copy link
Member

tsalo commented Aug 10, 2020

I don't know how/why I closed this. For some reason it won't let me reopen it though...

@emdupre
Copy link
Member

emdupre commented Aug 10, 2020

It says the ref/decision-tree branch was deleted. I can't seem to access it, either, and without it we can't re-open this PR. But all the changes are still on @handwerkerd 's branch (I just confirmed), so we could at least open a new PR !

@tsalo
Copy link
Member

tsalo commented Aug 10, 2020

oh thank goodness. phew!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants